-
Notifications
You must be signed in to change notification settings - Fork 983
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for base64-encoded hash values to 'get_file_hash' #1339
Conversation
The global template function 'get_file_hash' can now return a base64-encoded hash value when its 'base64' parameter is set to true. See discussion in #519.
SRI hash values must be base64-encoded.
@@ -95,18 +96,36 @@ fn compute_file_sha256(mut file: fs::File) -> result::Result<String, io::Error> | |||
Ok(format!("{:x}", hasher.finalize())) | |||
} | |||
|
|||
fn compute_file_sha256_base64(mut file: fs::File) -> result::Result<String, io::Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it will be cleaner to pass the base64 arg to those functions than to duplicate them all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about it but there were already 3 different functions for the 3 possible values of sha_type
so I opted for keeping it consistent.
Wouldn't it be even better to pass sha_type
and base64
to one unique function handling all the possible combinations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be even better to pass sha_type and base64 to one unique function handling all the possible combinations?
Even better!
Documentation updated. I also fixed the SRI issue in |
Is it still a WIP somehow or ok to merge? |
Hey @Keats. I didn't find the time to implement your suggestion of merging the different hashing functions into a single one (see #1339 (comment)). However, my PR can be merged in its current state so it's up to you. |
I'll fix it post-merge, thanks! |
Awesome! Thanks |
IMPORTANT: Please do not create a Pull Request adding a new feature without discussing it first.
The place to discuss new features is the forum: https://zola.discourse.group/
If you want to add a new feature, please open a thread there first in the feature requests section.
Sanity check:
Code changes
(Delete or ignore this section for documentation changes)
next
branch?If the change is a new feature or adding to/changing an existing one:
This PR followings up with the discussion in #519.